OCPBUGS-53408: wait for build and ensure OS image is actually new #4924
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
approved
cheesesashimi
force-pushed
the
zzlotnik/fix-mcd-and-node-controller
branch
2 times, most recently
from
cab64f7 to
8d2ede1
Compare
cheesesashimi
changed the title
openshift-ci-robot
added
jira/severity-important
|
@cheesesashimi: This pull request references Jira Issue OCPBUGS-53408, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
openshift-ci-robot
removed
the
jira/invalid-bug
|
@cheesesashimi: This pull request references Jira Issue OCPBUGS-53408, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
cheesesashimi
force-pushed
the
zzlotnik/fix-mcd-and-node-controller
branch
from
8d2ede1 to
e121461
Compare
|
/retest required |
|
@cheesesashimi: The The following commands are available to trigger optional jobs: Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
For pre-merge verification deployed the below CI job to verify the fix |
cheesesashimi
force-pushed
the
zzlotnik/fix-mcd-and-node-controller
branch
7 times, most recently
from
b398da2 to
c805f52
Compare
openshift-ci
bot
added
the
qe-approved
|
@cheesesashimi: This pull request references Jira Issue OCPBUGS-53408, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| // TODO: We should use the selectors from the build controller since they are | ||
| // well-tested and makes querying for this information significantly easier. | ||
| // Additionally, this should use listers instead of API clients in order to | ||
| // reduce the impact on the API server. |
There was a problem hiding this comment.
Suggestion: Make a Jira card to track this work todo.
pkg/controller/node/status.go
Outdated
| // TOOD: Handle this error better. | ||
| klog.Warningf("Error when checking isLayeredPool: %s", err) |
There was a problem hiding this comment.
Suggestion: Make a Jira card to track this work todo.
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest-required |
cheesesashimi
force-pushed
the
zzlotnik/fix-mcd-and-node-controller
branch
from
c805f52 to
51972a7
Compare
Occasionally, there is a delay between the time that a new rendered MachineConfig is produced and OCL begins a build. However, a couple of things happen in the interim: - The RenderController updates the MachineConfigPool. Because of the delay mentioned above, the NodeController begins updating all of the nodes with only the new rendered MachineConfig. The OS image remains the same because the NodeController is not ensuring that the image pullspec on the MachineOSConfig is the same as the MachineOSBuild. - Because of the work done to the MCD in openshift#4825, the original check that we had to determine whether the image pullspecs were the same is no longer present. Additionally, the logic change there makes it possible for an OS update to always occur whenever OCL is enabled, further bypassing that check. This fixes that by doing two things: 1. Update the Node Controller to ensure that the both the MachineOSBuild's MachineConfig reference matches the MCP's current rendered MachineConfig while also checking that the MachineOSConfig's image pullspec matches the MachineOSBuild's. In the situation where the MachineOSBuild's pullspec is empty, this check will fail and the Node Controller will requeue. 2. Update the MCD so that even when OCL is enabled, if the OS images are the same, the OS update process is skipped.
cheesesashimi
force-pushed
the
zzlotnik/fix-mcd-and-node-controller
branch
from
51972a7 to
57eee4a
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, isabella-janssen The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| l, err := ctrl.isLayeredPool(mosc, mosb) | ||
| if err != nil { | ||
| // TODO: Handle this error better. | ||
| klog.Warningf("Error when checking isLayeredPool: %s", err) |
There was a problem hiding this comment.
(question): I've checkout out the code and dived into this part and I was surprissed we must be dealing with an error that can only happen if this happens.
I was thinking about handling FeatureGates at a component level (mcd, mcs, controller), like, creating a dedicated struct that internally has a map of all the observed gates that can expose a method to check if the feature gate is enabled or not based on our rules and with centralized error handling. That way, if for example, the gates are not yet loaded, we can emit, in a single place, a log or an event. WDYT?
I'm proposing this not just because of your change, but because I see a similar pattern each time we need to check for a FeatureGate
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@cheesesashimi: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
bot
merged commit d5e8466
into
openshift:main
|
@cheesesashimi: Jira Issue OCPBUGS-53408: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-53408 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[ART PR BUILD NOTIFIER] Distgit: ose-machine-config-operator |
…-and-node-controller OCPBUGS-53408: wait for build and ensure OS image is actually new
…-and-node-controller OCPBUGS-53408: wait for build and ensure OS image is actually new
…-and-node-controller OCPBUGS-53408: wait for build and ensure OS image is actually new
…-and-node-controller OCPBUGS-53408: wait for build and ensure OS image is actually new
- What I did
Occasionally, there is a delay between the time that a new rendered MachineConfig is produced and OCL begins a build. However, a couple of things happen in the interim:
This fixes the behavior by making the following changes.
- How to verify it
This bug is difficult to reproduce. However, it seems to occur when multiple OCL-enabled pools are updating simultaneously.
- Description for the changelog
Wait for build and ensure OS image is actually new